Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clarify match block requirement for single mac call #159

Merged

Conversation

calebcartwright
Copy link
Member

cc @Aaron1011 re: rust-lang/rustfmt#4496 - think this minor clarification should cover the needed change but please let me know if you think anything additional is needed for the arm body flattening

@joshtriplett if you have time to review the text that'd be most appreciated as well!

@@ -661,7 +661,7 @@ match foo {
```

If the body is a single expression with no line comments and not a control flow
expression, then it may be started on the same line as the right-hand side. If
expression nor a macro call expression, then it may be started on the same line as the right-hand side. If
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't quite right. Both => macro_call!() and => { macro_call!() } are fine - the issue is automatically transforming one into the other.

For example, the code:

macro_rules! expr {
	() =>  { true };
}

macro_rules! stmt {
	() => { true; };
}

fn main() {
	let _val: bool = match true {
		true => expr!(),
		false => false,
	};

	match true {
		true => { stmt!() },
		false => {}
	}
}

will compile if rust-lang/rust#33953 is fixed. None of the match arms should be flattened/un-flattend, since that can change the meaning.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Aaron1011

Note that the Style Guide provides guidance/prescriptions and best practices for formatting in general, which rustc doesn't necessarily care about one way or the other. The compiler is fine with spaces vs. tabs and doesn't care whether a control flow expression in a match arm body is block-wrapped or not, but the Style Guide still provides explicit guidance on both fronts.

I understand that => macro_call!(), will be fine in some cases even after rust-lang/rust#33953 is fixed, but that doesn't necessarily preclude us from being able to provide guidance here (much in the same way that the compiler is fine with => if x { 1 } else { 2 }, but the Style Guide still says such a body, unrealistic as it may be, needs to be block-wrapped).

So I think the question for the Style Guide is whether it's worth prescribing block wrapping a single macro call expression as a general strategy to avoid running into parsing errors in cases where there's a trailing semi in the expansion.

If we don't want to go with something that strong, and still allow authors to write things like => println!("foo"), without rustfmt block wrapping it, that's fine too. Sure we could incorporate a general recommendation to be cognizant about block wrapping the body in cases where the expansion includes a semi which should provide sufficient cover for rustfmt to not flatten such cases.

None of the match arms should be flattened/un-flattend, since that can change the meaning.

Just to clarify, are you saying that block wrapping an arm body that has a single mac call expression could potentially fail to be semantics preserving (e.g. changing => macro_call!() to => { macro_call!() })? I can't quite see how that'd happen, though if so that'd be a separate issue we'd want to dig into as there are a few cases where rustfmt will do this today such as scenarios running up against max width with guards on the arm

Copy link
Member

@Aaron1011 Aaron1011 Nov 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, are you saying that block wrapping an arm body that has a single mac call expression could potentially fail to be semantics preserving

I was thinking that it could change the code from failing to compile (if the macro expands to a let statment, for example), to compiling. However, if the code already compiles, then introducing a block won't change the behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calebcartwright: I don't really have any opinion about what the style guide should say. As long as rust-lang/rustfmt#4496 is consistent with the style guide, I'll defer to your preferred wording.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually uses more forceful wording and would require additional work on the rustfmt side to explicitly reformat all such cases with a block wrapping, whereas rust-lang/rustfmt#4496 is just going to preserve the programmers original arm body structure.

Given the additional discussion in rust-lang/rustfmt#4496 about introducing a rustc warning in such cases, I'm actually now leaning towards changing the text here to make it a bit softer. The softer text would then align with the change rust-lang/rustfmt#4496, and although rustfmt couldn't be used to automatically correct/update the problematic cases, it will defer to devs and allow them to block wrap the arm bodies in need, and keep the ones that don't.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me. In general, I think rustfmt should be very cautious about making changes around macro invocations/definitions. I'm happy with accepting both the wrapped and unwrapped invocation, and adding something to the style guide about the pitfalls of the unwrapped case

@Aaron1011
Copy link
Member

@calebcartwright The new text looks good to me!

@calebcartwright
Copy link
Member Author

@calebcartwright The new text looks good to me!

Awesome, thanks so much really appreciate your help on this one @Aaron1011! Fighting some indentation issues from copy/pasting out of GH but will go ahead and merge once I get that sorted and then we can proceed with rust-lang/rustfmt#4496

@calebcartwright calebcartwright merged commit 4c4e3ef into rust-lang:master Nov 3, 2020
@calebcartwright calebcartwright deleted the match-rhs-mac-call-exprs branch November 3, 2020 03:26
@calebcartwright calebcartwright restored the match-rhs-mac-call-exprs branch January 18, 2023 17:17
@calebcartwright calebcartwright deleted the match-rhs-mac-call-exprs branch July 28, 2023 02:47
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 28, 2023
…oshtriplett

style-guide: don't flatten match arms with macro call

This pulls forward the gist of the text that was added to the style guide in rust-lang/style-team#159 to account for needing to tweak/soften rustfmt's behavior based on the style guide prescriptions.

There were a few options I considered, noted below, and although I don't particularly love any of them, I felt this was the lesser of the evils.

r? `@joshtriplett`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants